feat(#298): artifact upload endpoint + pluggable storage backend - #319

@copilot

Reviewed end-to-end. The architecture is clean — StorageBackend ABC + get_storage() factory, the #317-round-1 enum lessons are visibly applied (create_type=False from commit 1 in both migration and model), audit-log details matches the five-field contract that PR #314's assert.py will reconstruct from, and the test suite covers all the acceptance-criteria axes.

Two issues to address before merge, plus a small cosmetic.

🛑 1. Path traversal via user-controlled filename

filename flows from the form field straight into the storage key, which LocalFsBackend.save joins onto _root:

# backend/app/api/external_results.py
storage_key = f"{case_result_id}/{_uuid_module.uuid4().hex}/{filename}"

# backend/app/storage/local.py
def save(self, stream, *, key, content_type):
    dest = self._root / key
    dest.parent.mkdir(parents=True, exist_ok=True)
    with dest.open("wb") as fp:
        shutil.copyfileobj(stream, fp)

A filename = "../../../tmp/pwned.png" resolves outside _root. The intervening UUID-hex segment doesn't help — one extra .. undoes it. This is exploitable today on the local backend; once a real S3Backend lands it'd also affect S3 keys (less catastrophic but still bad hygiene).

Suggested fix (defense in depth — both layers):

# backend/app/api/external_results.py
import os, re

_FILENAME_RE = re.compile(r"^[A-Za-z0-9._-]{1,255}$")

safe_name = os.path.basename(filename)
if not _FILENAME_RE.fullmatch(safe_name):
    raise HTTPException(
        status_code=422,
        detail={
            "code": "validation_error",
            "message": "filename must match [A-Za-z0-9._-]{1,255} after basename stripping",
            "details": None,
        },
    )
storage_key = f"{case_result_id}/{_uuid_module.uuid4().hex}/{safe_name}"

# backend/app/storage/local.py — second line of defense
def save(self, stream, *, key, content_type):
    dest = (self._root / key).resolve()
    if not dest.is_relative_to(self._root.resolve()):
        raise ValueError(f"storage key {key!r} escapes artifact root")
    dest.parent.mkdir(parents=True, exist_ok=True)
    ...

Please add a test that filename = "../etc/passwd" (and a couple of other variants — null byte, backslash, absolute path) returns 422 and writes nothing.

⚠️ 2. Size enforcement isn't actually streaming

The PR description says the server "streams in 64 KiB chunks" and the spec doc says "The full body is never buffered into memory." Given the handler signature, that isn't true today:

async def upload_artifact(
    case_result_id: str = Form(...),
    kind: str = Form(...),
    filename: str = Form(...),
    file: UploadFile = File(...),
    ...
):

With Form(...) / File(...), FastAPI/Starlette fully parses the multipart body via python-multipart before the handler is invoked. By the time the chunk loop runs, file is already a SpooledTemporaryFile containing the entire upload (spilled to disk past ~1 MiB). The await file.read(_ARTIFACT_CHUNK_SIZE) loop is reading from that buffered file, not from the network stream.

Practical impact: a 10 GiB upload with BGSTM_ARTIFACT_MAX_BYTES=50 MiB is still fully received off the wire (and spooled to a Starlette-managed tmp file) before being rejected with 413. The DoS vector the limit was meant to close remains open.

The partial-write test passes anyway because the post-buffer chunk loop does write incrementally. The test can't distinguish "truly streaming" from "buffer first, then loop over the buffer" — so it's not catching the regression class it's documented to catch.

Two acceptable resolutions, pick whichever you prefer:

(A) Honest scope reduction (recommended for this PR)

• Update PR description and spec doc §f to say something like "Server enforces the size limit after the multipart body has been received and spooled by Starlette. True in-stream early-abort is tracked as a follow-up; deploy behind a reverse-proxy client_max_body_size for first-line DoS protection."
• Add a recommended client_max_body_size line to deploy docs.
• Reword the test docstring so it doesn't claim to catch buffer-vs-stream regressions.
• File a follow-up issue: "Artifact upload: enforce size limit during multipart parse, not after" — solution there is a streaming parser (streaming-form-data or python-multipart's MultipartParser) reading request.stream() and aborting mid-body.

(B) Real streaming in this PR

• Drop Form(...)/File(...) and parse request.stream() manually with a streaming multipart parser.
• Reject as soon as the running byte total crosses the limit; close the connection.
• Significantly larger change; keep for a follow-up unless you specifically want it now.

I'd lean (A) — the contract with the reporter doesn't change, the audit-log shape doesn't change, and we keep this PR focused.

3. Cosmetic: model/migration index-name mismatch

• Model declares index=True on case_result_id → SQLAlchemy default name ix_external_case_artifacts_case_result_id
• Migration creates explicit idx_external_case_artifacts_case_result_id

Functionally identical, but alembic revision --autogenerate against this state will propose drop+recreate. One-line fix — drop index=True from the column and rely on the explicit migration index, or align names via __table_args__ = (Index("idx_...", "case_result_id"),).

Out of scope for this PR (just confirming)

• Real S3 backend (stub correct, raises clearly ✅)
• Reporter bgstm:requirement annotations / requirement_external_ids — tracked in [v0.2] BGSTMReporter should transmit bgstm:requirement annotations and BGSTM should resolve external→UUID for case-result linking #316. Could add a one-liner reference under "Out of scope" in spec §f, but not blocking.

Once #1 and #2 are addressed I'm a 👍.

@copilot

Reviewed end-to-end. The architecture is clean — StorageBackend ABC + get_storage() factory, the #317-round-1 enum lessons are visibly applied (create_type=False from commit 1 in ...

All three issues addressed in commits a2adc37 and 3a8991f:

Issue 1 — Path traversal:

• _SAFE_FILENAME_RE now requires the first character to be alphanumeric (^[A-Za-z0-9][A-Za-z0-9._-]{0,254}$), which blocks ., .., ..., and dot-only names.
• Added safe_filename != filename guard — any filename containing a path separator (so it would be stripped by os.path.basename) is rejected with 422 rather than silently reused.
• LocalFsBackend.save() uses .resolve() + is_relative_to() as the second line of defense.
• 12 parametrized test cases: ../etc/passwd, /etc/shadow, subdir/file.png, null byte, backslash, 256-char name, ., .., ..., plus a direct LocalFsBackend key-escape test.

Issue 2 — Streaming scope: Went with option A. The 413 test docstring and spec doc §7 now accurately describe the Starlette post-buffer behavior and recommend a reverse-proxy client_max_body_size for first-line DoS protection.

Issue 3 — Index name: index=True replaced with explicit __table_args__ = (Index("idx_external_case_artifacts_case_result_id", "case_result_id"),) with a comment explaining it matches the migration name to prevent autogenerate noise.

@copilot

LGTM ✅ — all three review items resolved cleanly.

The #317-round-1 lessons are visibly applied: create_type=False on both Alembic ENUM and model-side Enum(...) from commit 1, manual .create(checkfirst=True) / .drop(checkfirst=True), explicit name="artifact_kind". Audit-log details carries all five fields the smoke assert.py in #314 will reconstruct from. get_storage() is a function, not a singleton — tests swap settings cleanly.

Two optional, non-blocking nits for whenever (don't hold up merge):

1. Spec §c.7 line 357 still says "size_bytes is counted while streaming the body" — mildly inconsistent with the "Note on buffering" 15 lines below. One-word swap to "while reading" if you're already in the file.
2. [v0.3] Artifact upload: enforce size limit during multipart parse (streaming, DoS-safe) #320 cross-reference isn't in the PR body or spec yet. A (tracked in #320) near the size-enforcement note would close the loop.

Separately, I noticed §d still documents "Artifacts — deduplication by SHA-256" but the implementation here doesn't hash or dedup. Pre-existing spec drift, not this PR's job — happy to file a follow-up issue to either implement or remove from the spec.

Approving. 🚀